Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bump YamlDotNet to 13.2.0 #433

Merged
merged 5 commits into from
Aug 16, 2023
Merged

Bump YamlDotNet to 13.2.0 #433

merged 5 commits into from
Aug 16, 2023

Conversation

mdanish-kh
Copy link
Contributor

@mdanish-kh mdanish-kh commented Aug 14, 2023 •
edited
Loading

This change now wraps all string values in the manifest around double quotes single quotes, we can choose from either but I went with one that looks cleaner. Single quotes don't parse escape sequences (not that it matters for our use case i think). (though imo this change makes the overall manifest look ugly now with everything quoted but that's just personal preference 😄)

Tested manually with

wingetcreate update Cockos.REAPER --urls "https://www.reaper.fm/files/6.x/reaper681_x64-install.exe|x64" "https://www.reaper.fm/files/6.x/reaper681-install.exe|x86"
wingetcreate update OpenAL.OpenAL --urls "https://www.openal.org/downloads/oalinst.zip"
Microsoft Reviewers: codeflow:open?pullrequest=#433

@mdanish-kh mdanish-kh requested a review from a team as a code owner August 14, 2023 07:19
@mdanish-kh mdanish-kh requested review from yao-msft and ryfu-msft and removed request for a team August 14, 2023 07:19
@Trenly
Copy link
Contributor

Trenly commented Aug 14, 2023 •
edited
Loading

(though imo this change makes the overall manifest look ugly now with everything quoted but that's just personal preference 😄)

I agree. Is there a switch to make it quote only values that need to be quoted?

Edit: I see there's quoteNecessaryStrings as part of the event emitter, which does contain Regex for float values. Perhaps we need to set that to true somehow?
https://github.com/aaubry/YamlDotNet/blob/bfba24411c54dee930cf87d93e313f6d724091b0/YamlDotNet/Serialization/EventEmitters/TypeAssigningEventEmitter.cs#L36-L46

Trenly
Trenly reviewed Aug 14, 2023
View reviewed changes
Copy link
Contributor

@Trenly Trenly left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found it (suggested above)

image

@mdanish-kh @Trenly
Update src/WingetCreateCore/Common/Serialization.cs
b287212 
Co-authored-by: Kaleb Luedtke <trenlymc@gmail.com>
@mdanish-kh
Copy link
Contributor Author

Thanks 🎉 I should've seen the whole implementation...🤦‍♂️

@ryfu-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ryfu-msft ryfu-msft merged commit 16564d3 into microsoft:main Aug 16, 2023
4 checks passed
@mdanish-kh mdanish-kh deleted the quotedYamlStrings branch August 16, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trenly Trenly Trenly left review comments

@ryfu-msft ryfu-msft ryfu-msft approved these changes

@yao-msft yao-msft Awaiting requested review from yao-msft yao-msft is a code owner automatically assigned from microsoft/oca

Assignees
No one assigned
Labels
Issue-Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

When Version is a Floating Point Number, it is Not Quoted
3 participants
@mdanish-kh @Trenly @ryfu-msft